Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.40%. Comparing base (06416e9) to head (a1f2667).
Report is 2 commits behind head on main.

@@            Coverage Diff             @@
##             main     #226      +/-   ##
==========================================
- Coverage   90.93%   90.40%   -0.53%     
==========================================
  Files          10        9       -1     
  Lines        1180     1167      -13     
==========================================
- Hits         1073     1055      -18     
- Misses        107      112       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Should we add one from from ClimaAtmos examples too, or is the test good enough?

Ideally, the Atmos unit tests should give a good signal to whether the examples work or not. So, if a change here does not break the Atmos unit tests but does break examples, then the Atmos tests need to be expanded on.

Otherwise, where do we stop? Do we also add the long runs? This is like what @sriharshakandala was talking about. Please confirm, @Sbozzolo.

Should we add one from from ClimaAtmos examples too, or is the test good enough?

Ideally, the Atmos unit tests should give a good signal to whether the examples work or not. So, if a change here does not break the Atmos unit tests but does break examples, then the Atmos tests need to be expanded on.

I absolutetly agree with this.

ClimaTimesteppers and ClimaDiagnostics don't depend on Thermodynamics, so you can remove them.

There is an additional coupler test you can add to run a few steps of AMIP. It will probably fail right now because SST and SIC artifacts became too big that cannot be downloaded automatically. You can start by downloading those files automatically (as we do here), but I will also produce a smaller version that can be downloaded automatically in the future.

There is an additional coupler test you can add to run a few steps of AMIP. It will probably fail right now because SST and SIC artifacts became too big that cannot be downloaded automatically. You can start by downloading those files automatically (as we do here), but I will also produce a smaller version that can be downloaded automatically in the future.

IMO, we should stick to the unit tests, which should have a tightly coupled signal to the experiments, and encourage/support a culture of thorough unit tests.

There is an additional coupler test you can add to run a few steps of AMIP. It will probably fail right now because SST and SIC artifacts became too big that cannot be downloaded automatically. You can start by downloading those files automatically (as we do here), but I will also produce a smaller version that can be downloaded automatically in the future.

IMO, we should stick to the unit tests, which should have a tightly coupled signal to the experiments, and encourage/support a culture of thorough unit tests.

ClimaCoupler is very different: unit tests in ClimaCoupler are only for the coupling infrastructure but have no physics in it (with the coupler intentially left as a thin and agnostic package). All the physics is in the ClimaEarth experiment, which runs AMIP. Turning ClimaEarth into its own package is part of our longer term project goals, but that is not currently being worked on.

ClimaCoupler is very different: unit tests in ClimaCoupler are only for the coupling infrastructure but have no physics in it (with the coupler intentially left as a thin and agnostic package).

There are physics-related pieces in the ClimaCoupler test suite. For example, here's a function with physics currently in the test suite:

function FieldExchanger.update_sim!(sim::TestAtmos, fields, _)
    (; F_turb_ρτxz, F_turb_energy, F_turb_moisture) = fields
    ρ_int = sim.integrator.ρ
    @. sim.integrator.p.energy_bc = -(F_turb_energy)
    @. sim.integrator.p.ρq_tot_bc = -F_turb_moisture
    @. sim.integrator.p.uₕ_bc = -(F_turb_ρτxz / ρ_int) # x-compoennt only for this test
end

All the physics is in the ClimaEarth experiment, which runs AMIP. Turning ClimaEarth into its own package is part of our longer term project goals, but that is not currently being worked on.

If something in the ClimaEarth experiment relies on some sort of physics, that should also be included in the unit test suite. In fact, if it's only exercising components from Atmos, then we should catch that even earlier, in the Atmos unit tests. I would rather us be consistent about this, than make exceptions that people don't understand and are subject to change (first its AMIP, next its CMIP). As @sriharshakandala said during our discussion, running all upstream experiments is not scalable, and I agree. Our unit tests should have a good signal that the experiments will run, if they don't then the unit tests should be expanded on.

Consider that the coupler team is composed by 1 person at the moment. ClimaEarth is currently living inside ClimaCoupler because there is no person-power to work on it and make it a separate package with its unit tests. This is also because it was decided that this is not a priority for our project right now.

I absolutely agree with you that unit tests should be enough and we should expand on them, but if you don't include the ClimaEarth test I can tell you that you are missing an important piece that is important to us.

I absolutely agree with you that unit tests should be enough and we should expand on them, but if you don't include the ClimaEarth test I can tell you that you are missing an important piece that is important to us.

Which pieces? I can help add them to the coupler unit test

Perhaps the root of the issue comes from the fact that we've developed the coupler so far with component model-agnostic coupling infrastructure in src/, and some component model-specific adapter code in experiments/ClimaEarth/ - for example extensions of getter and setter functions we need to update fluxes. The long term goal is to move all the model-specific code in experiments/ClimaEarth/ into the respective component repositories, but that isn't the state we're at currently.

The ClimaCoupler unit tests do cover our source code, e.g. interface functions, flux calculations, etc, but they don't cover the component model-specific experiment code. This is why a short downstream AMIP experiment is helpful to inform us if e.g. any interfaces have broken. I agree that we should have thorough unit tests for all of our code, but ClimaCoupler is in a tricky situation right now and as Gabriele said it will require some considerable work to get it to where we want it to be.

Also, I'm not sure what is meant by adding a unit test for the AMIP experiment - would this look like running a coupled simulation for a couple of timesteps, or something different?

@charleskawczynski @Sbozzolo

Ok, fair point about considerable remaining work. I'm fine with adding this test temporarily. Is there a relatively future-proof way to add it to the downstream tests here?

Ok, fair point about considerable remaining work. I'm fine with adding this test temporarily. Is there a relatively future-proof way to add it to the downstream tests here?

I think the best way is to add the same test Atmos is using (added here). It's been in the test suite there for a few months and as far as I know it hasn't caused problems